fix: skip unhealthy devices in Fit() for all non-nvidia backends - #2260
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUnhealthy-device checks now apply across multiple device backends and allocation paths. Rejected devices increment ChangesDevice health allocation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device/awsneuron/device.go`:
- Around line 316-317: Update continuousDeviceAvailable and graphSelect to
propagate whether a contiguous multi-device selection was blocked by an
unhealthy device instead of collapsing it into an empty result. In
AWSNeuronDevices.Fit, record common.CardNotHealth alongside the existing
multi-device common.NumaNotFit result when that health failure occurs, and add
coverage for an unhealthy device within a requested contiguous range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aec2cbf3-0af5-41a1-b2fc-b6026acd9a0c
📒 Files selected for processing (15)
pkg/device/amd/device.gopkg/device/ascend/device.gopkg/device/ascend/device_test.gopkg/device/awsneuron/device.gopkg/device/biren/device.gopkg/device/cambricon/device.gopkg/device/enflame/device.gopkg/device/enflame/gcu.gopkg/device/hygon/device.gopkg/device/iluvatar/device.gopkg/device/kunlun/device.gopkg/device/kunlun/vdevice.gopkg/device/metax/device.gopkg/device/mthreads/device.gopkg/device/vastai/device.go
|
Hi @Eshiv-Pandey! I'm still pretty new to the HAMi codebase, so please take this as a curious observation, not a review 🙂 I was reading through this PR — really like the consistency it brings, having every backend skip unhealthy devices makes a lot of sense. While going through it I noticed the if !dev.Health check gets added across quite a few backends, but the test case looks like it's only for ascend. I could easily be missing where the others are covered — but if they aren't yet, might it be worth adding a small CardNotHealth case for the rest too (amd, awsneuron, biren, cambricon, enflame, hygon, iluvatar, kunlun, metax, mthreads, vastai)? Just a thought, to help guard against regressions down the line. If that'd be useful, I'd genuinely be happy to pitch in and help add them — would love to contribute here. |
|
@veyron-kairo you are not wrong here, but this one is just adding consistency here nothing new. If you can open the files changed tab, you will see that it's the same 4-5 loc repeating in each backend. still if maintainers ask for adding test cases for each backend separately, it would be much easier for me to mimic those. You can take up other issues ..like the one mentioned by code rabbit in this pr |
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
this is the sweep we asked for in #2241, thanks.
7b3d11d to
f3a82a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device/amd/device_test.go`:
- Around line 554-580: Update the shared test loops for the unhealthy-device
cases so that when wantFit is false they also assert result[AMDDevice],
result[BirenDevice], result[CambriconMLUDevice], result[EnflameGCUDevice],
result[HygonDCUDevice], result[MetaxGPUDevice], result[MthreadsGPUDevice],
result[VastaiDevice], and result[AWSNeuronDevice] have wantLen 0 and wantDevIDs
empty. Apply this validation in pkg/device/amd/device_test.go lines 554-580,
pkg/device/biren/device_test.go lines 771-798,
pkg/device/cambricon/device_test.go lines 1042-1069,
pkg/device/enflame/gcu_test.go lines 576-603, pkg/device/hygon/device_test.go
lines 1135-1162, pkg/device/metax/device_test.go lines 825-852,
pkg/device/mthreads/device_test.go lines 1000-1027,
pkg/device/vastai/device_test.go lines 800-827, and
pkg/device/awsneuron/device_test.go lines 863-893; preserve the existing success
assertions and failure-reason checks.
In `@pkg/device/iluvatar/device_test.go`:
- Around line 592-618: The fit test table and shared assertion must verify
rejection reasons as well as fit status. Add a wantReason field to the “fit
fail: CardNotHealth” case with CardNotHealth, then retain and compare the third
Fit return value against wantReason in the shared test logic.
In `@pkg/device/kunlun/vdevice_test.go`:
- Around line 529-534: Update the positive test fixtures for “idle device always
fits” and “shared device with matching average memory” to explicitly set Health:
true in their device.DeviceUsage values, so they remain accepted by FitVXPU.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56af29ca-b57e-4f91-9364-b7b2bdb7e57b
📒 Files selected for processing (24)
pkg/device/amd/device.gopkg/device/amd/device_test.gopkg/device/awsneuron/device.gopkg/device/awsneuron/device_test.gopkg/device/biren/device.gopkg/device/biren/device_test.gopkg/device/cambricon/device.gopkg/device/cambricon/device_test.gopkg/device/enflame/device.gopkg/device/enflame/gcu.gopkg/device/enflame/gcu_test.gopkg/device/hygon/device.gopkg/device/hygon/device_test.gopkg/device/iluvatar/device.gopkg/device/iluvatar/device_test.gopkg/device/kunlun/device.gopkg/device/kunlun/vdevice.gopkg/device/kunlun/vdevice_test.gopkg/device/metax/device.gopkg/device/metax/device_test.gopkg/device/mthreads/device.gopkg/device/mthreads/device_test.gopkg/device/vastai/device.gopkg/device/vastai/device_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/device/kunlun/vdevice.go
- pkg/device/hygon/device.go
- pkg/device/iluvatar/device.go
- pkg/device/kunlun/device.go
- pkg/device/mthreads/device.go
- pkg/device/amd/device.go
- pkg/device/biren/device.go
- pkg/device/enflame/device.go
- pkg/device/cambricon/device.go
- pkg/device/metax/device.go
- pkg/device/awsneuron/device.go
- pkg/device/vastai/device.go
- pkg/device/enflame/gcu.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device/iluvatar/device_test.go`:
- Line 439: Populate the wantReason field in every failing table entry for the
memory, core, type, UUID, exhaustion, mutex, and partial-allocation cases in the
device Fit tests, using the exact non-empty “1/1 ...” reason returned by Fit;
preserve the existing assertion comparing Fit’s reason at the wantReason check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a901ad8c-5178-4ed5-8eee-b673ee742a92
📒 Files selected for processing (11)
pkg/device/amd/device_test.gopkg/device/awsneuron/device_test.gopkg/device/biren/device_test.gopkg/device/cambricon/device_test.gopkg/device/enflame/gcu_test.gopkg/device/hygon/device_test.gopkg/device/iluvatar/device_test.gopkg/device/kunlun/vdevice_test.gopkg/device/metax/device_test.gopkg/device/mthreads/device_test.gopkg/device/vastai/device_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/device/awsneuron/device_test.go
- pkg/device/hygon/device_test.go
- pkg/device/biren/device_test.go
- pkg/device/vastai/device_test.go
- pkg/device/amd/device_test.go
- pkg/device/enflame/gcu_test.go
- pkg/device/kunlun/vdevice_test.go
- pkg/device/metax/device_test.go
8891004 to
e01314b
Compare
daa6005 to
9a7f1d2
Compare
After adding the !dev.Health early-exit to Fit() across all non-nvidia backends, two test files had devices without Health: true set: - enflame/device_test.go: TestFit_SelectProfileByRequest, TestFit_SelectProfileByMemoryCoreRequest, and TestFit_MutexRejectsUsedDevice used DeviceUsage structs with Health defaulting to false, causing the health gate to fire before the logic under test. Set Health: true on those devices. - iluvatar/device_test.go: Test_Fit existing failure-path cases had no wantReason set (empty string), but the test loop now checks reason unconditionally. Added the correct expected reason strings: CardInsufficientMemory, CardTypeMismatch, ExclusiveDeviceAllocateConflict. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
…k added to Fit() The !dev.Health early-exit added to Fit() and continuousDeviceAvailable() caused graphSelect test fixtures to fail because DeviceUsage.Health defaults to false in Go, making every device appear unhealthy. Add Health: true to all DeviceUsage fixtures in: - awsneuron/device_test.go: Test_graphSelect - kunlun/device_test.go: Test_graphSelect, Test_graphSelectVXPU, TestKunlunVDevices_Fit_Mutex Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
8d4d91f to
80d4167
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… lost in rebase The rebase onto latest master dropped two changes from the original PR: 1. The !dev.Health check in enflame/device.go Fit() was lost 2. Health: true on test fixtures in enflame/device_test.go was lost This commit restores both and adds a TestFit_UnhealthyDeviceRejected test to bring enflame/device.go patch coverage to 100%. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
|
@mesutoezdil can you have a look now? have rebased since pr #2238 is merged, also have fixed the merge conflicts. |
Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
Resolve conflict in pkg/device/awsneuron/device_test.go by keeping both the CardNotHealth test case (ours) and the NumaNotFit with multiple devices test case (theirs from Project-HAMi#2292). Apply makeAWSDeviceUsage helper to the merged NumaNotFit case for consistency with the refactor. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
- Run gofmt -w to fix formatting - Replace for i := 0; i < total; i++ with for i := range 16 to satisfy the modernize linter (range over int, Go 1.22+) Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
|
/assign @Shouren |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Eshiv-Pandey, FouoF, Shouren The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
The nvidia backend already guards against unhealthy devices in
Fit()usingdev.Health. The same check was missing from every other backend: amd, awsneuron, biren, cambricon, enflame (device.go + gcu.go), hygon, iluvatar, kunlun (device.go + vdevice.go), metax, mthreads, and vastai.For loop-based backends, the check is added at the top of the device loop before any other checks, mirroring the nvidia pattern exactly. For graph-select backends (kunlun), the check is added inside the fitness functions (FitXPU, FitVXPU). For awsneuron, it is added in both the single-device loop path and inside continuousDeviceAvailable() which is used for multi-device topology-aware selection.
Follows up on #2241 which fixed the same gap for ascend.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
GetNodeDevices()(amd, cambricon, mthreads, awsneuron, enflame/gcu) are unaffected in practice today but now match the contract for future use.Health: true and are unaffected.Does this PR introduce a user-facing change?:
No. Unhealthy devices are now skipped during scheduling instead of being allocated, consistent with the nvidia backend.
AI assistance disclosure:
I used an AI tool for codebase navigation and code generation. The changes were reviewed and verified by me.
Summary by CodeRabbit
Bug Fixes
Tests